-
-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
203-selectabletextblock-does-not-work #219
Conversation
2) SelectableTextBlock.axaml in gallery textblock 3) default template for SelectableTextBlock.axaml
Warning Rate limit exceeded@jinek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces mouse support and interaction capabilities across multiple console implementations in the Consolonia framework. The changes primarily involve adding two new boolean properties, The modifications extend beyond just adding properties. For instance, the Additionally, the Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/Consolonia.Core/InternalHelpers/CommonInternalHelper.cs (1)
Line range hint
8-11
: Fix IsNearlyEqual implementationThe current implementation using
CompareTo
doesn't handle floating-point comparison correctly. The TODO comment also indicates this is problematic.Consider this implementation instead:
-public static bool IsNearlyEqual(this double value, double compareTo) -{ - //todo: strange implementation for this name - return value.CompareTo(compareTo) == 0; -} +public static bool IsNearlyEqual(this double value, double compareTo, double epsilon = 1e-10) +{ + return Math.Abs(value - compareTo) < epsilon; +}
🧹 Nitpick comments (6)
src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml (1)
8-9
: Consider UX implications of right-click selectionUsing right-click for text selection (
SelectOnMouseRightUp="True"
) is an unconventional choice that might confuse users, as right-click typically opens context menus. Consider:
- Using left-click for selection (standard behavior)
- Adding a context menu for additional actions
Would you like assistance in implementing a more conventional selection behavior?
src/Consolonia.Core/InternalHelpers/CommonInternalHelper.cs (2)
6-6
: Reconsider making CommonInternalHelper publicChanging the class from
internal
topublic
exposes implementation details that were previously encapsulated. This could make future maintenance more difficult as other assemblies might start depending on these utilities.Consider:
- Creating a separate public API if these utilities are needed externally
- Keeping implementation details internal to maintain encapsulation
Line range hint
18-23
: Modernize null checkingThe
NotNull
method could be simplified using C# 8.0's null-coalescing operator.Consider this more concise implementation:
-public static T NotNull<T>(this T? t) where T : struct -{ - if (t == null) - throw new ConsoloniaException("Value is not expected to be null"); - return t.Value; -} +public static T NotNull<T>(this T? t) where T : struct + => t ?? throw new ConsoloniaException("Value is not expected to be null");src/Consolonia.Core/Infrastructure/IConsole.cs (1)
25-26
: Add XML documentation for mouse support propertiesSimilar to
SupportsComplexEmoji
, these new properties should have XML documentation explaining their purpose and implications.Add documentation like this:
+/// <summary> +/// Indicates whether this console implementation supports mouse input. +/// </summary> bool SupportsMouse { get; } +/// <summary> +/// Indicates whether this console implementation supports mouse movement tracking. +/// </summary> bool SupportsMouseMove { get; }src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml (1)
23-25
: Consider improving the SelectableTextBlock demonstrationThe current implementation has a few potential issues:
- The pre-defined selection range (17-26) might be confusing as it's not immediately clear what text is being selected
- The demonstration could be more effective with clearer examples
Consider these improvements:
- <SelectableTextBlock SelectionStart="17" SelectionEnd="26"> - Selectable text: Loreum ipsum dolor sit amet, consectetur adipiscing elit. Vivamus magna. Cras in mi at felis aliquet congue. Ut a est eget ligula molestie gravida. Curabitur massa. Donec eleifend, libero at sagittis + <SelectableTextBlock> + Try selecting this text with your mouse! This demonstrates the text selection functionality. + </SelectableTextBlock> + <!-- Example with pre-selected text --> + <SelectableTextBlock SelectionStart="0" SelectionEnd="15"> + Selected text: This text starts pre-selected to demonstrate the selection API. + </SelectableTextBlock>src/Consolonia.NUnit/UnitTestConsole.cs (1)
Line range hint
193-193
: Consider documenting the unused MouseEventThe
MouseEvent
is defined but never raised since mouse support is disabled. Consider adding a comment explaining this design decision.+ // MouseEvent is defined for interface compatibility but never raised as mouse support is disabled public event Action<RawPointerEventType, Point, Vector?, RawInputModifiers> MouseEvent;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Consolonia.Core/Dummy/DummyConsole.cs
(1 hunks)src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs
(1 hunks)src/Consolonia.Core/Infrastructure/IConsole.cs
(1 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs
(1 hunks)src/Consolonia.Core/InternalHelpers/CommonInternalHelper.cs
(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryTextBlock.axaml
(1 hunks)src/Consolonia.NUnit/UnitTestConsole.cs
(1 hunks)src/Consolonia.PlatformSupport/CursesConsole.cs
(2 hunks)src/Consolonia.PlatformSupport/WindowsConsole.cs
(1 hunks)src/Consolonia.Themes/Templates/Controls/AllControls.axaml
(1 hunks)src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs
(1 hunks)src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml
(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml
[warning] 7-7:
"[Xaml.StaticResourceNotResolved] Resource 'ThemeActionBackgroundBrush' is not found" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml(7,40)
🔇 Additional comments (12)
src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml (1)
6-7
: Verify ThemeActionBackgroundBrush resource availability
The static analysis indicates that ThemeActionBackgroundBrush
resource is not found. This resource needs to be defined or merged from another resource dictionary to avoid runtime binding errors.
✅ Verification successful
ThemeActionBackgroundBrush resource is properly defined and available
The resource ThemeActionBackgroundBrush
is defined in the base colors file TurboVisionDefaultColors.axaml
as a ConsoleBrush
with the color ForestGreen
. The resource is also referenced across multiple theme files and controls, indicating it's a core part of the theming system. The binding in SelectableTextBlock.axaml
is valid and will work correctly at runtime.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ThemeActionBackgroundBrush definition
rg -l "ThemeActionBackgroundBrush" --type xaml
Length of output: 75
Script:
#!/bin/bash
# Search for ThemeActionBackgroundBrush definition in all files
# Using ripgrep without file type filter since .xaml extension isn't recognized
rg -l "ThemeActionBackgroundBrush"
Length of output: 977
Script:
#!/bin/bash
# Find all .xaml files in the repository to understand resource structure
fd -e xaml
Length of output: 12
Script:
#!/bin/bash
# Look for resource dictionaries that might contain brush definitions
rg -l "ResourceDictionary|<Style|<Brush"
Length of output: 4188
Script:
#!/bin/bash
# Check the definition of ThemeActionBackgroundBrush in color resource files
rg "ThemeActionBackgroundBrush" src/Consolonia.Themes/Templates/TurboVisionDefaultColors.axaml -A 2 -B 2
Length of output: 382
Script:
#!/bin/bash
# Check if the SelectableTextBlock template merges the required resource dictionaries
rg "MergedDictionaries|Source=" src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml -A 2 -B 2
Length of output: 110
🧰 Tools
🪛 GitHub Check: build
[warning] 7-7:
"[Xaml.StaticResourceNotResolved] Resource 'ThemeActionBackgroundBrush' is not found" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml(7,40)
src/Consolonia.Core/Infrastructure/IConsole.cs (1)
25-26
: Verify mouse support implementation consistency
The new mouse support properties should be consistently implemented across all console implementations that handle the MouseEvent
.
Also applies to: 39-40
✅ Verification successful
Mouse support properties are consistently implemented across console implementations
All console implementations that handle mouse events have consistent implementation of the SupportsMouse
and SupportsMouseMove
properties:
WindowsConsole
: Fully supports mouse with both properties set totrue
CursesConsole
: Supports mouse (true
) but not mouse move (false
)- Other implementations (
UnitTestConsole
,DefaultNetConsole
,DummyConsole
): Both properties set tofalse
and don't implement mouse event handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for MouseEvent implementations
ast-grep --pattern 'class $_ : IConsole {
$$$
event Action<RawPointerEventType, Point, Vector?, RawInputModifiers> MouseEvent
$$$
}'
Length of output: 140
Script:
#!/bin/bash
# Search for files containing IConsole implementations
fd -e cs | xargs rg -l "class.*:.*IConsole"
# Search for files containing MouseEvent
fd -e cs | xargs rg "MouseEvent"
# Search for SupportsMouse and SupportsMouseMove implementations
fd -e cs | xargs rg "(SupportsMouse|SupportsMouseMove)"
Length of output: 6085
src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs (2)
15-17
: LGTM: Property registration is well-structured
The attached property registration follows best practices for Avalonia property system.
30-37
: LGTM: Event handler management is robust
The implementation correctly handles event subscription lifecycle and uses proper null checking patterns.
src/Consolonia.Core/Dummy/DummyConsole.cs (1)
37-38
: LGTM: Mouse support properties correctly implemented
The properties align with the dummy console's purpose and are consistently placed with other support flags.
src/Consolonia.Themes/Templates/Controls/AllControls.axaml (1)
7-7
: LGTM: Resource inclusion is properly placed
The SelectableTextBlock resource is logically placed after the base TextBlock resource.
src/Consolonia.Core/Infrastructure/DefaultNetConsole.cs (1)
51-52
: LGTM! Mouse support properties correctly implemented.
The implementation correctly indicates that this console doesn't support mouse interactions, which is appropriate for a default console implementation.
src/Consolonia.NUnit/UnitTestConsole.cs (1)
43-44
: LGTM! Mouse support properties correctly implemented for test console.
The implementation correctly indicates that this test console doesn't support mouse interactions.
src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
40-41
: LGTM! Mouse support properties are well-defined.
The abstract properties are correctly defined and follow the same pattern as other capability flags in the class.
src/Consolonia.PlatformSupport/WindowsConsole.cs (1)
65-66
: LGTM! Mouse support properties match implementation capabilities.
The properties correctly indicate full mouse support, which aligns with the existing comprehensive mouse event handling implementation in the class.
src/Consolonia.PlatformSupport/CursesConsole.cs (2)
96-101
: LGTM! Mouse button mappings updated to match terminal conventions.
The button mappings have been correctly updated to align with common terminal emulator behavior:
- Button2 → Middle button
- Button3 → Right button
119-120
: Verify mouse move support implementation.
There appears to be an inconsistency:
SupportsMouseMove
is set tofalse
- However,
MouseEventFlagTranslator
includes mapping forReportMousePosition
toRawPointerEventType.Move
Consider either:
- Setting
SupportsMouseMove
totrue
if mouse movement events are actually supported - Removing the
ReportMousePosition
mapping if mouse movement is not supported
✅ Verification successful
Let me check the actual implementation of mouse move handling in these files.
Mouse move events are supported in Windows but not in Curses
The code analysis reveals an important implementation detail:
- WindowsConsole properly implements mouse move events through
EventFlags.MouseMoved
- CursesConsole maps
ReportMousePosition
toRawPointerEventType.Move
, but this appears to be unused sinceSupportsMouseMove
is false
The current implementation is correct because:
- The
ReportMousePosition
mapping in CursesConsole is likely a remnant or placeholder - Setting
SupportsMouseMove = false
correctly indicates that Curses implementation doesn't actively support mouse movement tracking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if mouse move events are actually used in the codebase
# Search for mouse move event handling
rg -l "RawPointerEventType.Move" --type cs
# Search for specific usage of ReportMousePosition
ast-grep --pattern 'ReportMousePosition'
Length of output: 232
Script:
#!/bin/bash
# Check the implementation details of mouse move handling
rg "RawPointerEventType.Move" -B 5 -A 5 --type cs
# Check MouseEventFlagTranslator implementation
rg "MouseEventFlagTranslator" -B 5 -A 15 --type cs
Length of output: 8574
src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs
(1 hunks)src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Themes/Templates/Controls/SelectableTextBlock.axaml
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs (1)
Learnt from: jinek
PR: jinek/Consolonia#219
File: src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs:21-25
Timestamp: 2024-12-19T21:20:43.248Z
Learning: When mouseMove is supported by IConsole, the SelectableTextBlock handles selection normally without needing an additional subscription for right-click pointer up events in SelectTextWithPointerUpExtension.
🔇 Additional comments (2)
src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs (2)
21-25
: LGTM! Mouse support check is correctly implemented
The early return when !supportsMouse || supportsMouseMove
is correct as:
- We need mouse support for this feature to work
- When mouse move is supported, SelectableTextBlock handles selection normally without needing this extension
30-37
: LGTM! Event handler management is well implemented
The implementation follows best practices:
- Type checking
- Proper event handler cleanup
- Conditional subscription
src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs
Outdated
Show resolved
Hide resolved
src/Consolonia.Themes/Templates/Controls/Helpers/SelectTextWithPointerUpExtension.cs
Outdated
Show resolved
Hide resolved
TargetType="SelectableTextBlock"> | ||
<Setter Property="SelectionBrush" | ||
Value="{DynamicResource ThemeActionBackgroundBrush}" /> | ||
<Setter Property="helpers:SelectTextWithPointerUpExtension.SelectOnMouseLeftUp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next level magic.
I haven't ever created a mixin extension like this. Xaml is so freaking powerful as a model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Its supposed to be simpler with "behaviours", they manage lifetime etc, but Avalonia does not have them builtin, i dont want to include that assembly to core level packages of consolonia.
No description provided.